NM-273: Vnat pool assignments fix#3926
Conversation
|
Tenki Code Review - Complete Files Reviewed: 4 By Severity:
This PR refactors Virtual NAT (VNAT) pool allocation by centralizing the logic in Files Reviewed (4 files) |
There was a problem hiding this comment.
Overview
The PR consolidates VNAT pool allocation logic — previously duplicated between migrate/migrate.go and pro/logic/egress.go — into exportable functions in logic/networks.go (AllocateUniqueVNATPool, AllocateUniquePoolFromFallback, NthSubnet). It also fixes a response encoding bug in updateNetwork and cleans up the migration function.
Positive Changes
updateNetworkresponse fix (controllers/network.go:826): The handler previously returned the raw clientpayload, missing server-managed fields (ID,CreatedAt, VNAT settings). Switching tonetOld— which is mutated in-place byUpdateNetwork()— correctly returns the persisted DB state.- Refactoring: Centralizing
AllocateUniquePoolFromFallback,NthSubnet, and related helpers avoids duplication betweenmigrate/andlogic/. - Uniqueness guarantee:
AllocateUniqueVNATPoolnow loads all existing network pools before allocating, preventing conflicts at network creation time.
Issues Found
bigIntToIP missing overflow guard (logic/networks.go:374)
The bigIntToIP function in logic/networks.go pads short byte slices but does not truncate when big.Int.Bytes() returns more bytes than expected (overflow). The identical function in pro/logic/egress.go includes this truncation guard. Without it, an overflowed IPv4 address could produce a nil result from To4(), causing NthSubnet to return a nil IP — which the pool allocation loop correctly skips, but means a valid pool slot is silently lost.
Migration no longer gated by feature flag (migrate/migrate.go:79)
The PR removes the check if !logic.GetFeatureFlags().EnableOverlappingEgressRanges { return } from initializeVirtualNATSettings(). This means the migration now populates VNAT fields for all Pro networks on startup, regardless of whether the overlapping egress feature is enabled. If intentional (pre-populating for future feature activation), this should be documented with a comment. If unintentional, the guard should be restored.
Describe your changes
Provide Issue ticket number if applicable/not in title
Provide testing steps
Checklist before requesting a review